Repository navigation
refactor: walk a subtree in steps, with its termination rules in one place - #46
Merged
Merged
Conversation
ilyam8
added this pull request to stack #38
October 9, 2026 22:46
ilyam8
force-pushed
the
engine-walk-loop
branch
2 times, most recently
from
October 11, 2026 09:46
40d8000 to
5d6cac1
Compare
WalkFunc, Walk, WalkAll, BulkWalk and BulkWalkAll move from gosnmp.go into walk.go unchanged, as the request methods live in request.go. No behavior change.
…arkWalk All pass on the walk before its restructuring: - sibling-arc: an answer whose name extends the root's last arc (.1.3.6.1.2.1.2.2.1.20.1 when walking .1.3.6.1.2.1.2.2.1.2) is outside the subtree and ends the walk. - user-code/walkfn-sets-max-repetitions: a walkFn that changes MaxRepetitions does not change the walk's later GetBulk requests. - user-code/walkfn-sets-app-opts-c: a walkFn that sets the AppOpts 'c' option does not turn off the walk's increasing check. BenchmarkWalk runs WalkAll and BulkWalkAll, as the Netdata Agent's SNMP collector does, over a 100-row table column on the in-memory agent.
…e place walk was one labeled loop whose break and continue carried the decisions, with one case per error status. It is now: - walk: the loop. It sends the walk's request, has visit judge the answer, and acts on the returned walkStep: ask for the objects after the answer's last name, get the root (a leaf), or end. - walkRoot: the default root and the leading dot. - walker.request: GetBulk, GetNext or Get of the walk's OID. - walker.visit: the rules for an answer: an empty answer, the error statuses (one range, TooBig to InconsistentName, logged by their generated names), exception values, the subtree, the increasing check, walkFn. - walker.outside: an object outside the subtree: the leaf fallback, the root itself, the end. - inSubtree: strings.HasPrefix(name, root+".") without building the prefix for every object. MaxRepetitions and AppOpts are still read once per walk. walkFn stays a parameter: as a field of walker it escapes with the client and moves WalkAll's closure and results to the heap. The walk's log lines are guarded by Logger.enabled(). Two dead conditions are gone: the NoSuchInstance check of the root branch (the exception check returns first) and the request-type condition of the AppOpts 'c' check (walk runs only GetNext and GetBulk). Each pinned walk bug has a known-bug comment where it happens. No behavior change: goldens and logger output unchanged.
A change that makes the engine loop (a regression or a mutation probe) used to grow the scenario's transcript until the test timed out, holding gigabytes when several such runs had no timeout. The transcript now fails its test once it passes 5000 lines, about a hundred times the longest scenario (53 lines), so such a change fails in milliseconds.
All pass on the walk before its restructuring: - leaf/root-echo: a first answer naming the root itself makes the walk take the root for a leaf and get it. - leaf/get-answer-in-subtree: after the leaf fallback, an answer inside the subtree is walked and the walk sends a Get for that name (known bug: net-snmp's snmpwalk makes one Get). - not-increasing/app-opts-c-nil: the AppOpts 'c' key turns the increasing check off whatever its value, nil included. - error-status/19-unknown-empty: an answer with an unnamed error status and no varbinds ends the walk as an empty answer does.
From the step's reviews: - The error-status rule names its range, tooBig to inconsistentName, in its condition and its comment instead of relying on the noError case before it. - The leaf-root known bug sits where it happens: an endOfMibView first answer, or SNMPv1's noSuchName, ends the walk before the fallback. - A new known-bug comment where the fallback switches to Get: the walk keeps sending Gets after it. - The default root's comment cites X.690 alone; RFC 2578 section 7.1.3 sets no minimum number of arcs. - The fallback cites gosnmp/gosnmp#78, which added it; the subtree example names the excess names correctly. No behavior change: goldens and logger output unchanged.
ilyam8
force-pushed
the
engine-walk-loop
branch
from
October 11, 2026 09:52
5d6cac1 to
8455686
Compare
vkalintiris
approved these changes
Oct 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restructures the walk behind
Walk,WalkAll,BulkWalkandBulkWalkAll. No behavior or exported API change.walkwas one labeled loop whosebreakandcontinuecarried the decisions, with onecaseper error status and the subtree, exception and increasing rules interleaved in one loop body.gosnmp.gointowalk.gounchanged.walkis the loop: it sends the walk's request, hasvisitjudge the answer, and acts on an explicitwalkStep(ask for the objects after the answer's last name, get the root as a leaf, or end).walker.visitholds the rules for an answer in order: an empty answer, the error statuses (one range, tooBig to inconsistentName, logged by their generated names), exception values, the subtree, the increasing check, walkFn.walker.outsidedecides at a value outside the subtree (the leaf fallback, the root itself, the end);walkRootgives the default root and the leading dot;inSubtreereplacesstrings.HasPrefix(name, root+".")without building the prefix for every value.MaxRepetitionsandAppOptsare still read once per walk.walkFnstays a parameter: as a field ofwalkerit escaped with the client and movedWalkAll's closure and results to the heap.Logger.enabled(). Two dead conditions are gone: theNoSuchInstancecheck of the root branch (the exception check returns first) and the request-type condition of the AppOptsccheck (the walk runs only GetNext and GetBulk).Each known walk bug has one known-bug comment where it happens, including one found in review: after the leaf fallback the walk keeps sending Gets.
The test commits pin what mutation probes and two reviews found unobserved, each passing on the previous walk: a sibling arc that shares the root's text (gosnmp/gosnmp#78, #93), walkFn changing
MaxRepetitionsorAppOptsmid-walk, a first answer echoing the root, the leaf fallback's Get answered inside the subtree,AppOpts{"c": nil}, and an empty answer with an unnamed error status.BenchmarkWalkrunsWalkAllandBulkWalkAllover a 100-row column. The engine test harness now fails a scenario whose transcript passes 5000 lines, so a change that makes the engine loop fails in milliseconds instead of growing memory until the test timeout.Testing
go test ./...on darwin/arm64 andGOARCH=386(386 at the restructuring commit; the later commits change tests and comments);-race; the FIPS 140-only tests; golangci-lint v2.14.0 (incl.end2end): 0 issues;GOOS=windows go vet.defaultand 4 that change only the Logger call (caught by the Logger comparison).WalkAll2432 -> 2229 andBulkWalkAll400 -> 295 (the guarded per-answer log line and the per-value prefix);BenchmarkGetidentical.